reject RBNF plural rules whose divisor overflows to zero#4004
reject RBNF plural rules whose divisor overflows to zero#4004rootvector2 wants to merge 1 commit into
Conversation
|
|
Please also check if this is a problem in Java -- ideally by porting the unit test. |
grhoten
left a comment
grhoten
left a comment
There was a problem hiding this comment.
I confirmed that the Number Format Tester also throws a "/ by zero" exception, which uses ICU4J. So the change needs to be mirrored to both.
These changes are just modifying how the failure is surfaced. I find it unlikely that this issue would have surfaced through typical usage of RBNF in any language. I think only some French variants use the divisor like this, and it's only for numbers 60-99. While it's a valid issue, it's unlikely to be encountered outside of synthetic rule making with large numbers.
Please follow the directions provided by Markus to get this change mergeable.
|
Confirmed it's the same bug in ICU4J: Agree this only changes how the failure surfaces, and you'd really only hit it with synthetic large-divisor rules rather than real RBNF data. Rejecting at construction just matches the existing zero-divisor guards instead of dividing by zero at format time. Restored and filled out the PR template. On the Jira ticket: I'm not on the team so I can't file in the tracker directly. I can submit it through the bug page, or if it's easier for you to open one I'll prefix the title and commits with the number. |
rootvector2
force-pushed
the
rbnf-zero-divisor
branch
from
3df5922 to
e2bb077
Compare
|
Hooray! The files in the branch are the same across the force-push. 😃 ~ Your Friendly Jira-GitHub PR Checker Bot |
|
Squashed to a single commit. Both bot checks stay red until the title has a ticket ID (the |
| @@ -504,6 +504,13 @@ NFRule::extractSubstitutions(const NFRuleSet* owner, | |||
| } | |||
| rulePatternFormat = formatter->createPluralFormat(pluralType, | |||
| ruleText.tempSubString(endType + 1, pluralRuleEnd - endType - 1), status); | |||
| if (U_SUCCESS(status) && getDivisor() == 0) { | |||
There was a problem hiding this comment.
This should probably be <= 0 in both implementations. A divisor should be a positive number. Bad things would probably happen if it were negative.
There was a problem hiding this comment.
Makes sense, changed both to <= 0. Depending on radix the wrap can land on a negative value instead of 0, so rejecting anything non-positive is right. TestDividedByZero and TestDivisorOverflow still pass.
A slash divisor descriptor with a $(cardinal,...)$ body sets the plural rule pattern without creating a modulus or integral-part substitution, so the zero-divisor checks in those substitutions never run. When radix^exponent overflows 64 bits the power helper wraps to 0 and doFormat divides the number by it: SIGFPE in ICU4C, ArithmeticException in ICU4J. Reject the rule with U_PARSE_ERROR / IllegalStateException when the plural format is built with a non-positive divisor (overflow can also wrap negative), mirroring the existing substitution guards. Tests added for both C++ (TestDividedByZero) and Java (TestDivisorOverflow).
rootvector2
force-pushed
the
rbnf-zero-divisor
branch
from
e2bb077 to
6c84bff
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
3 participants
A slash divisor descriptor with a
$(cardinal,...)$body setsrulePatternFormatbut creates no modulus or integral-part substitution, so the divisor checks in those substitution constructors never run. Whenradix^exponentoverflows 64 bits the power helper wraps to 0 (util64_powin ICU4C,NFRule.powerin ICU4J), anddoFormat()then divides the formatted number by that zero divisor: integer divide by zero, SIGFPE on x86-64 in C,ArithmeticException: / by zeroin Java.extractSubstitutionsnow rejects the rule as soon as the plural format is built andgetDivisor() == 0, mirroring the existing zero-divisor guards in the modulus and integral-part substitutions. ICU4C surfacesU_PARSE_ERROR; ICU4J throwsIllegalStateException, matching how those substitutions already reject a zero divisor.Confirmed the same crash exists in ICU4J (the Number Format Tester /
numbers.jspthrows/ by zero), so the fix and the test are mirrored to both. The C++TestDividedByZeroand the new JavaTestDivisorOverflowboth fail without the fix and pass with it.Checklist